Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add HTTPS upgrade checks #23029

Merged
merged 2 commits into from
May 8, 2024
Merged

Add HTTPS upgrade checks #23029

merged 2 commits into from
May 8, 2024

Conversation

cuba
Copy link
Contributor

@cuba cuba commented Apr 11, 2024 •
edited
Loading

Resolves brave/brave-browser#36408

Security review: https://github.com/brave/reviews/issues/1593

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

  1. Check that http://example.com upgrades correctly. Back and forth navigation buttons work as expected
  2. Check that some made up website (ex: http://jfdskllfsjlks.com) does not upgrade. Back and forth buttons work as expected.
  3. Check the links on the following page are upgraded: https://shivankaul.com/brave/https-upgrades/embedded_link.html

Note: I don't know of any sites that do exist in http but not in https. If you are able to come across one please test it.

@cuba cuba requested a review from a team as a code owner April 11, 2024 13:06
@cuba cuba force-pushed the js/https-exception-service-ios branch 2 times, most recently from a683903 to 57f5d6e Compare April 12, 2024 17:22
Brandon-T
Brandon-T requested changes Apr 12, 2024
View reviewed changes
...rave-ios/Sources/Brave/Frontend/Browser/BrowserViewController/BVC+WKNavigationDelegate.swift
@@ -599,6 +599,26 @@ extension BrowserViewController: WKNavigationDelegate {
let response = navigationResponse.response
let responseURL = response.url
let tab = tab(for: webView)
let isInvalid: Bool
if let httpResponse = response as? HTTPURLResponse {
isInvalid = httpResponse.statusCode >= 400
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure about this >= 400 check? What happens if the response is a 000?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so:

brave-core/chromium_src/chrome/browser/ssl/https_upgrades_interceptor.cc

Line 44 in c4daecc

if (headers && headers->response_code() >= 400) { \

@Brandon-T
Copy link
Contributor

Brandon-T commented Apr 12, 2024 •
edited
Loading

Needs a manual sec review for secure icon state when upgrading from http to https: brave/brave-browser#36951 (comment).

@cuba cuba requested a review from Brandon-T April 18, 2024 15:59
@cuba cuba force-pushed the js/https-exception-service-ios branch from 57f5d6e to 74a3491 Compare April 18, 2024 16:20
@cuba cuba mentioned this pull request May 1, 2024
Merged
24 tasks
@cuba
Add https strict mode and interstitial (#23337)
d1795f9 
Add http upgrade strict mode and interstitial
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented May 7, 2024

[puLL-Merge] - brave/brave-core@23029

Description

This PR adds support for the HTTPS Upgrades feature in Brave iOS. It allows upgrading HTTP connections to HTTPS and blocking HTTP connections that can't be upgraded, with a user-facing setting to control the behavior.

Changes

Changes

  • ios/BUILD.gn, ios/app/BUILD.gn, ios/brave-ios/Package.swift: Added new files and targets related to HTTPS upgrades
  • ios/app/brave_core_main.h, ios/app/brave_core_main.mm: Added HTTPSUpgradeExceptionsService to BraveCoreMain
  • ios/brave-ios/App/iOS/Delegates/AppState.swift: Registered HTTPBlockedHandler to handle HTTP blocked interstitial page
  • ios/brave-ios/Sources/Brave/Assets/Interstitial Pages/Pages/HTTPBlocked.html: Added interstitial page HTML for HTTP blocked connections
  • ios/brave-ios/Sources/Brave/Frontend/Browser/BrowserViewController/*: Added logic to upgrade HTTP connections, handle exceptions, and show interstitial
  • ios/brave-ios/Sources/BraveShared/Extensions/URLExtensions.swift, ios/brave-ios/Sources/Shared/Extensions/URLExtensions.swift: Added methods related to HTTP blocked page URLs
  • ios/brave-ios/Sources/BraveShields/*: Added HTTPSUpgradeLevel enum and related strings/prefs for HTTPS upgrades setting
  • ios/browser/api/https_upgrade_exceptions/*, ios/browser/application_context/*: Added HTTPSUpgradeExceptionsService to handle upgrade exceptions

Security Hotspots

  1. Carefully review the logic for upgrading HTTP to HTTPS and allowing exceptions, to avoid bypassing the security benefits
  2. Ensure the HTTP blocked interstitial page does not introduce any XSS or HTML injection vulnerabilities
  3. Verify that the HTTPSUpgradeExceptionsService is storing exceptions securely and not leaking any cross-site information

Overall this is a security-enhancing feature, but the implementation should be audited to ensure it does not introduce any new vulnerabilities around SSL/HTTPS. The core logic around upgrading and allowing exceptions is the key area to focus on.

@cuba cuba merged commit e4391d5 into master May 8, 2024
19 checks passed
@cuba cuba deleted the js/https-exception-service-ios branch May 8, 2024 19:48
@github-actions github-actions bot added this to the 1.68.x - Nightly milestone May 8, 2024
@ShivanKaul ShivanKaul mentioned this pull request Aug 23, 2024
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stoletheminerals stoletheminerals stoletheminerals approved these changes

@Brandon-T Brandon-T Brandon-T approved these changes

Assignees

@cuba cuba

Labels
needs-security-review puLL-Merge
Projects
None yet
Milestone
1.68.x - Release
Development

Successfully merging this pull request may close these issues.

[iOS] HTTPS by default feature on iOS
3 participants
@cuba @Brandon-T @stoletheminerals